Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using atomic counters on shared containers #473

Merged
merged 13 commits into from
May 16, 2023

Conversation

lemire
Copy link
Member

@lemire lemire commented May 8, 2023

Some of our users rely on "copy-on-write" (default to disabled). A bitmap with the copy-on-write flag
set to true might generate shared containers. A shared container is just a reference to a single
container with reference counting (we keep track of the number of shallow copies). If you copy shared
containers over several threads, this might be unsafe due to the need to update the counter concurrently.
Thus for shared containers, we use reference counting with an atomic counter. If the library is compiled
as a C library (the default), we use C11 atomics. Unfortunately, Visual Studio does not support C11
atomics at this times (though this is subject to change). So we use Windows intrinsics when we have to.

Fixes #472

@lemire lemire mentioned this pull request May 8, 2023
@lemire
Copy link
Member Author

lemire commented May 10, 2023

Ah. So we can't have atomics under Visual Studio, unless the library is rebuilt as a C++ library... because Visual Studio does not support C11 atomics...?

@lemire
Copy link
Member Author

lemire commented May 10, 2023

I am adding the Windows support via Windows intrinsic functions, see #474

1 similar comment
@lemire
Copy link
Member Author

lemire commented May 10, 2023

I am adding the Windows support via Windows intrinsic functions, see #474

* Using Windows intrinsics.

* Some fixes.

* Removing silly comment.
@lemire
Copy link
Member Author

lemire commented May 10, 2023

Now with Windows intrinsics.

@Dr-Emann
Copy link
Member

I gave a try at centralizing things, in #475, let me know what you think, @lemire

* Use typedef/inline functions to centralize atomic ref count ops

* __STDC_NO_ATOMICS__ only matters if we're c11+

I _think_ this will fix all the msvc compile issues

* MSVC can probably use Interlocked functions even in c++

* _Interlocked ops actually take a signed argument

* Ack. uint32->uint32_t

Probably was rust brain, thinking `u32`
tests/CMakeLists.txt Outdated Show resolved Hide resolved
@lemire lemire merged commit 264b188 into master May 16, 2023
ttaylorr added a commit to ttaylorr/CRoaring that referenced this pull request May 22, 2023
The assertion added via 264b188 (Using atomic counters on shared
containers (RoaringBitmap#473), 2023-05-15) does not explicitly include <assert.h>
causing GCC to fail compilation when invoked with
`-Werror=implicit-function-declaration`.

In C++ this is OK, since there `assert()` refers to its macro
definition. But in C the only way to get `assert()` is by including
<assert.h>.

Signed-off-by: Taylor Blau <[email protected]>
lemire pushed a commit that referenced this pull request May 23, 2023
* portability.h: include missing `<assert.h>`

The assertion added via 264b188 (Using atomic counters on shared
containers (#473), 2023-05-15) does not explicitly include <assert.h>
causing GCC to fail compilation when invoked with
`-Werror=implicit-function-declaration`.

In C++ this is OK, since there `assert()` refers to its macro
definition. But in C the only way to get `assert()` is by including
<assert.h>.

Signed-off-by: Taylor Blau <[email protected]>

* src: avoid old-style function declarations

A handful of functions with argument arity zero fail when the compiler
is invoked with `-Werror=old-style-definition`. Work around these by
explicitly declaring the parameter list as `void` to clarify that these
functions expect zero arguments.

Signed-off-by: Taylor Blau <[email protected]>

* roaring_array.h: declare `ra_get_container()`

This function dates all the way back to 3d12719 (some more untested
code, plus some files that had not been tracked hitherto, 2016-01-19),
but never had a prototype.

This causes compilers building with `-Werror=missing-prototypes` to fail
compilation. Declare a prototype for that function in the corresponding
header accordingly.

Signed-off-by: Taylor Blau <[email protected]>

* containers/bitset.h: declare `bitset_container_union_nocard()`

In a similar spirit as the previous commit, declare a function prototype
for `bitset_container_union_nocard()` which rounds out the existing set
of function prototypes for the macro expansion of:

    BITSET_CONTAINER_FN(union, |, _mm256_or_si256, vorrq_u64)

Signed-off-by: Taylor Blau <[email protected]>

* containers/bitset.h: declare `bitset_container_intersection_nocard()`

In a similar spirit as the previous commits, declare a function
prototype for `bitset_container_intersection_nocard()` which rounds out
the existing set of function prototypes for the macro expansion of:

    BITSET_CONTAINER_FN(intersection, &, _mm256_and_si256, vandq_u64)

Signed-off-by: Taylor Blau <[email protected]>

---------

Signed-off-by: Taylor Blau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

COW thread safety
2 participants